Semver-major changes #173

coreyfarrell · 2019-08-11T07:05:08Z

I can break this into smaller PR's if that makes it easier to review. This enables 100% coverage, adds osx to the travis config (lutimes polyfills can only be tested on osx). Note the first 3 commits in this PR are already submitted to #169.

One thing I'm not thrilled about is how fs.promises.open is implemented. The first call to fs.promises.open may be slightly delayed while the system finishes patching that function. Specifically we need the native C++ FileHandle and the JavaScript class FileHandle from lib/internal/fs/promises.js. The native C++ FileHandle is acquired through the process.binding('fs').FileHandle which is deprecated. The only way to get class FileHandle is by calling the original await fs.promises.open and finding the constructor of the result. Access to the native C++ FileHandle is required to support automagic closing of unclosed fd's on garbage collection. Access to the JS class FileHandle is required to allow us to extend the base class, otherwise we would have to reimplement all functions and this would potentially have other negative effects.

Fixes #152
Fixes #160
Fixes #167
Fixes #170

Breaking Changes

Require node.js 8
Drop legacy polyfilling
Properly handle DEP0013 based on node.js runtime version (warn in node.js 8/9, throw in 10+)

Features

Patch fs.promises where available (node.js 10+). Note first access to require('graceful-fs').promises accesses fs.promises so in node.js 10 it will trigger the experimental feature warning.

Fixes

Honor autoClose: false in WriteStream.open
Prevent accidental monkey-patching of fs.ReadStream / fs.WriteStream
Prevent multiple patching of process.cwd, process.chdir
Emit ready event upon stream open

Refractors

Modernize code
Unify ENFILE/EMFILE patches
Unify chmod/chown patches
Unify stream patches
Drop TEST_GRACEFUL_FS_GLOBAL_PATCH, test with gfs.gracefulify
Eliminate GRACEFUL_FS_PLATFORM, move Windows polyfills to separate files to allow direct testing
Move lutimes patches to separate file to allow ignoring coverage when unsupported
Use fs.mkdtempSync in tests (this will allow parallel testing once Support for repeating test files with multiple env settings tapjs/tapjs#586 is resolved)

Importing a fresh copy of graceful-fs then using `require('fs').close` would only initiate retry of the queue from the first copy of graceful-fs, so needed retries associated with subsequent instances of graceful-fs would not be executed.

Some changes in the previous commit are not needed in v4.x which will never be tested under nyc. The only thing needed is the ability for the next version of graceful-fs to reset fs.close / fs.closeSync.

This was for an issue with versions of node.js that are EOL. Node.js commits which fixed this issue: * v4.6.2+ - nodejs/node@3d6f107a2f * v6.8.0+ - nodejs/node@07d97f4f3e * v7.0.0+ - nodejs/node@3d6f107a2f Also remove associated testing.

Properly handle DEP0013 in ENFILE/EMFILE patched functions depending on node.js version.

* Add autoClose support to WriteStream.open * Prevent accidental monkey-patching of fs.*Stream

Properly handle DEP0013 in patched fs.close.

Process callback argument to ensure proper handling if missing.

* Create windows-rename-polyfill.js * Eliminate process.env.GRACEFUL_FS_PLATFORM * Modernize windows-rename-polyfill test

* Prefer const * Space before function parentheses * Remove unused args

coreyfarrell · 2019-08-14T13:23:22Z

So far the only reaction to nodejs/node#29109 has been negative. I'm not sure it's a great idea to publish the promises feature without a path out of hack-land. I'm inclined to say lets hold this for a bit to see what happens over in node.js.

coreyfarrell · 2019-08-14T15:06:26Z

Additionally I'd like to see where things go with some active PR's against node.js fs streams. In particular I think graceful-fs@5.0.0 should conditionally use whatever options are eventually provided by nodejs/node#29083, the less we override the better.

* Avoid process.binding * Monkey-patch promises FileHandle.close to call retry * Do not use promisify * Eliminate delay on first open

brodycj · 2019-11-24T07:17:25Z

package.json

+    "glob": "^7.1.4",
+    "import-fresh": "^3.1.0",
+    "nyc": "^14.1.1",
+    "rimraf": "^2.6.3",


Seems to be up to rimraf@3.0.0, as I proposed in PR #177

brodycj · 2019-11-24T07:22:12Z

Any news on this one?

coreyfarrell · 2019-11-24T14:35:28Z

@brodybits on hold for nodejs/node#29083. I think the last CI blocker was just fixed in the PR so hopefully that can be merged soon. Once it's merged and (hopefully) backported to 13.x I'll be updating here to use that feature when possible and will refresh latest versions of all dependencies.

ronag · 2019-12-18T15:07:46Z

FYI nodejs/node#29083 is now merged. It was merged as semver-minor and I believe it should be included in node 13.6.0 or soon thereafter.

brodycj · 2019-12-18T17:24:41Z

Patch fs.promises where available (node.js 10+).

I would favor dropping Node.js 8 support since it is near EOL.

coreyfarrell · 2019-12-19T13:32:06Z

@ronag thanks for the update. I'll update this once fs stream overrides are in a release (I will need a check against process.versions.node to detect that feature).

@brodybits node.js support will be up to @isaacs. Worth noting the detection of fs.promises is relatively simple, dropping node.js 8 would not result in significantly decreased complexity, so really it's only a question of possibly holding back development dependencies (this is not a concern for end users).

brodycj · 2019-12-19T15:48:41Z

Thanks @coreyfarrell. My idea was that dropping Node.js 8 would reduce burden of future updates for the next 1-2 years or so. Just an idea:)

zkochan · 2020-06-07T18:08:15Z

Why does it take so long to merge this?

coreyfarrell added 30 commits August 5, 2019 16:54

Create/use global shared retry queue.

ea9e6bc

Importing a fresh copy of graceful-fs then using `require('fs').close` would only initiate retry of the queue from the first copy of graceful-fs, so needed retries associated with subsequent instances of graceful-fs would not be executed.

Add test flag allowing reset of fs.close / fs.closeSync

43ba0b1

Remove changes not needed in v4.x

a91bd18

Some changes in the previous commit are not needed in v4.x which will never be tested under nyc. The only thing needed is the ability for the next version of graceful-fs to reset fs.close / fs.closeSync.

Require Node.js 8, update devDependencies

3cf751e

Drop legacy-streams.js

99b67e5

Remove v0.6.2 backport fix for fs.lchmod

91ed07a

Remove fs.*stat* gid/uid fixup

cb8ca43

This was for an issue with versions of node.js that are EOL. Node.js commits which fixed this issue: * v4.6.2+ - nodejs/node@3d6f107a2f * v6.8.0+ - nodejs/node@07d97f4f3e * v7.0.0+ - nodejs/node@3d6f107a2f Also remove associated testing.

Drop fallback for util.debuglog

d70ba36

Unify ENFILE/EMFILE error checks

7e52d7f

Properly handle DEP0013 in ENFILE/EMFILE patched functions depending on node.js version.

Create unified ReadStream / WriteStream patching function

be2498f

* Add autoClose support to WriteStream.open * Prevent accidental monkey-patching of fs.*Stream

Modernize / simplify clone.js

2bb23c3

Modernize graceful-fs.js

82736a8

Properly handle DEP0013 in patched fs.close.

Modernize win32 fs.rename polyfill

5af296f

Process callback argument to ensure proper handling if missing.

Modernize fs.read / fs.readSync polyfills

99851ef

Unify chown/chmod polyfills

da0b918

Remove TEST_GRACEFUL_FS_GLOBAL_PATCH, test with gfs.gracefulify

8e34dfc

Split win32 fs.rename patcher to separate source file

67dfc90

* Create windows-rename-polyfill.js * Eliminate process.env.GRACEFUL_FS_PLATFORM * Modernize windows-rename-polyfill test

Create noop.js

b28befc

Move lutimes polyfills to separate file

c0da35c

Collect code coverage

01fdd0b

Full coverage

6d8dae1

Modernize tests, make style consistent

8853947

Prevent multiple patching of process

ad402e9

Fix testing for future versions of nyc

159c190

Use fs.mkdtempSync in tests

438495b

Implement fs.promises patching

1743a28

Add support for withFileTypes to readdir

23ee88b

Fix code style issues that slipped in

3e4abee

* Prefer const * Space before function parentheses * Remove unused args

Emit ready event on stream open

295c972

Fix node.js 8 test issues

d108be7

coreyfarrell added 2 commits August 11, 2019 02:59

Fix testing for node.js 10, set enumerable for gfs.promises

32f2a75

Fix win32 testing

4d8ddeb

coreyfarrell mentioned this pull request Aug 13, 2019

Straight-forward way to reimplement fs.promises.open nodejs/node#29109

Closed

Hide fs.close and fs.closeSync previous symbols

7a7822a

Improve promises functionality

1483484

* Avoid process.binding * Monkey-patch promises FileHandle.close to call retry * Do not use promisify * Eliminate delay on first open

brodycj reviewed Nov 24, 2019

View reviewed changes

brodycj mentioned this pull request Nov 24, 2019

rimraf@3 update in devDependencies #177

Open

brodycj mentioned this pull request Dec 18, 2019

[MAJOR] update devDependencies #176

Closed

zkochan mentioned this pull request Jun 4, 2020

mz/fs pnpm/pnpm#2609

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Semver-major changes #173

Semver-major changes #173

coreyfarrell commented Aug 11, 2019

coreyfarrell commented Aug 14, 2019

coreyfarrell commented Aug 14, 2019

brodycj Nov 24, 2019

brodycj commented Nov 24, 2019

coreyfarrell commented Nov 24, 2019

ronag commented Dec 18, 2019 •

edited

Loading

brodycj commented Dec 18, 2019

coreyfarrell commented Dec 19, 2019

brodycj commented Dec 19, 2019

zkochan commented Jun 7, 2020

Semver-major changes #173

Are you sure you want to change the base?

Semver-major changes #173

Conversation

coreyfarrell commented Aug 11, 2019

Breaking Changes

Features

Fixes

Refractors

coreyfarrell commented Aug 14, 2019

coreyfarrell commented Aug 14, 2019

brodycj Nov 24, 2019

Choose a reason for hiding this comment

brodycj commented Nov 24, 2019

coreyfarrell commented Nov 24, 2019

ronag commented Dec 18, 2019 • edited Loading

brodycj commented Dec 18, 2019

coreyfarrell commented Dec 19, 2019

brodycj commented Dec 19, 2019

zkochan commented Jun 7, 2020

ronag commented Dec 18, 2019 •

edited

Loading